Force-push:

• Rebase on main after drivers: video: gc2145: Add Chip ID 0x2145 #87872

Thanks ! It will help drivers not need to define their own read / write register functions. Each drivers has different type of register address and value:

• Some are 16-bit address registers and need to read/write 8-bits data
• Some are 16-bit address registers and need to read/write 16-bit data

Does the CCI library will work in all these cases ? Per my observation, Linux also has CCI library but I don't know why (video) drivers still define and use their own read / write function (?).

Will we have also modify register functions (to modify only some bit through a mask) ?

Yes the goal is to offer a replacement for all the custom write functions for compatible sensors.

Supported address sizes:

• 8-bit address
• 16-bit address

Supported Data sizes:

• 8-bit data
• 16-bit data (big/little endian) sent one byte at a time
• 24-bit data (big/little endian) sent one byte at a time (sensors define arbitrary size for their register)
• 32-bit data (big/little endian) sent one byte at a time (the max supported, 64-bit registers needs to be split)

Supported operations:

• Read one register into a 32-bit variable (32-bit, 24-bit, 16-bit, 8-bit alike, like in Linux)
• Write one register from a 32-bit variable (32-bit, 24-bit, 16-bit, 8-bit alike, like in Linux)
• Write multiple registers using a struct video_reg table
• Write one bitfield described by a value and a mask. Should it be named video_modify_cci_reg() instead of video_write_cci_field()?

BTW, in order to use the CCI read/write/modify functions, we need to convert all the register addresses to 32-bit:

#define IMX335_REG8(addr)  ((addr) | VIDEO_REG_ADDR16_DATA8)
#define IMX335_REG16(addr)  ((addr) | VIDEO_REG_ADDR16_DATA16_LE)
#define IMX335_REG24(addr)  ((addr) | VIDEO_REG_ADDR16_DATA24_LE)

For drivers that uses dozens or hundreds of registers, it seems rather a significant amount of memory. Should it be better if we take these additional information as function parameters (maybe separate functions for 8bit, 16bit, 24 bit registers) and/or as a config (LE or BE does not change on a platform) instead of encoding all of them in the address parameter ?

we need to convert all the register addresses to 32-bit

The flags are forced o be 32-bit here but good to clear any doubt through an obvious syntax.

dozens or hundreds of registers

For instance, for the galaxycore,gc2145 driver, I skipped the CCI functions for this purpose. On Linux, it is always 32-bit address and 32-bit data fields, but ROM is typically more abundant on a Linux-capable chip than a microcontroller.

The first implementation (unpublished) used 7 variants (write8, write16le, write16be, write24le, write24be, write32le, write32le), although this does not permit to define table of registers with many .

Switching to another strategy that implementation was wrapping the "table" declaration like this:

#define VIDEO_REG_ADDR16_DATA24(addr, reg) \
        { (addr) + 0, (reg) >> 16 }, \
        { (addr) + 1, (reg) >> 8 }, \
        { (addr) + 2, (reg) >> 0 }

struct { uint16_t addr; uint8_t data; } init_regs[] = {
        VIDEO_REG_ADDR16_DATA24(0x3008, 0xaabbcc),
        IMX335_REG8(0x3016, 0),
        IMX335_REG16(0x3014, 8),
        {0},
};

For 16-bit addresses and 24-bit registers:

before: 8 bytes

[ addr0 | addr1 |      |      | data0 | data1 | data2 |       ]  4 + 4

after: 16 bytes:

[ addr0 | addr1 | data0 |      ]  2 + 2
[ addr0 | addr1 | data1 |      ]  2 + 2
[ addr0 | addr1 | data2 |      ]  2 + 2
[ addr0 | addr1 | data3 |      ]  2 + 2

For 8-bit addresses and 8-bit registers

A frequent pathological case for small image sensors (8-bit address) with big tables of undocumented register...

before: 8 bytes:

[ addr0 | addr1 |      |      | data0 |      |      |      ]  4 + 4

after: 2 bytes:

[ addr0 | data0 ]  1 + 1

Currently, the struct video_reg is uint32_t addr; uint32_t data;.

There could be variants to cover the compromises to wrap these frequent "big data tables" use-cases, this is only 2 extra types to add:

struct video_reg16 { uint16_t addr; uint8_t data; }; -> 4 bytes, 2× better
for 16-bit addresses (same as uint16_t addr; uint16_t data; in practice due to alignment)

struct video_reg8 { uint8_t addr; uint8_t data; }; -> 2 bytes, 4× better
for 8-bit addresses

config (LE or BE does not change on a platform) instead of encoding all of them in the address parameter

It was a compromise to reduce the API size non-V4L2 embedded developers need to face to avoid too much domain-specific knowledge required for writing video drivers:

1. In #define SENSORNAME_REG16(addr) (addr | FLAGS_THEY_DECIDE), the addition of the "type flags" is defined by the developer themselves. A trick to actively push the undestanding of "CCI magic" to non-V4L2 developers.
2. By having only one flag, there is no room for mistake, only valid flags are defined, and not so many to maintain.
3. A 16-bit address that encodes a 2-bit "address size" flag becomes a 32-bit variable, with 14-bit unused anyway.

If another compromise is possible, glad to hear about it, I probably did not think about it.
I tried to find an 'OK' balance but I do not know what the best answer is!

@kartben could you have a look at this PR so that we could merge it if ok since it is a dependency of several other PR. Thanks ;)

The blocking PR was about a copilot review, which we can trigger independently, so let's go with it then dismiss the stale change request once Copilot review is addressed. :)

If no progress is made, the assignee (maintainer) has the right to dismiss stale, unrelated or irrelevant change requests by reviewers giving the reviewers a minimum of 1 business day to respond and revisit their initial change requests or start the escalation process.
-- https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-technical-escalation

Force-push:

• Just rebasing on latest main for now.

Force-push:

• Apply Copilot proposed typo-fixes

Tested via #87937 just rebased on top of it.
Politely dismissing kartben review as I assume this is addressed by now.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud